Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add/experimental grid block all controls #3444

Closed
wants to merge 22 commits into from

Conversation

enejb
Copy link
Contributor

@enejb enejb commented May 1, 2021

Adds the submodule Block experiements.

Related PR: Automattic/block-experiments#187

iOS: wordpress-mobile/WordPress-iOS#16423

Android PR:
wordpress-mobile/WordPress-Android#14578

To test:
run

  1. git submodule update
  2. Start metro. npm run start
  3. Add the layout grid block.
  4. Follow the rest of the instruction on Mobile: Add simple layout grid controls Automattic/block-experiments#187

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes - No release notes just yet since we are not releasing the new block to the public)

@@ -23,6 +23,9 @@ addAction( 'native.render', 'gutenberg-mobile', ( props ) => {
setupJetpackEditor(
props.jetpackState || { blogId: 1, isJetpackActive: true }
);
if ( __DEV__ ) {
require( './block-experiments-setup' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why this needs to be required dynamically. Could we instead import { registerBlock as registerLayoutGridBlock } from '../block-experiments/blocks/layout-grid/src'; here, and invoke it under __DEV__? If this is for the incidental idempotency given by require.cache, maybe we can explicitly make this idempotent by setting a flag in the file-level closure here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that a require is necessary because metro doesn't support require imports just yet. See #2582 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment refers to why we would use require() instead of import(), which are both dynamic. What I'm wondering is, why are we aiming for this to be dynamic? I.e. why not a static import at the top of the file, and dynamically register the block under __DEV__?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I think part of the reason why I included it like this is so that we don't load the js script when the flag is not set. but since we are not downloading the js I guess this part doesn't really matter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cool to explore something like this down the road for lazy-loading / code splitting.

@@ -0,0 +1,5 @@
// This file is to set up the jetpack/layout-grid block that currently lives in block-experiments/blocks/layout-grid

import { registerBlock } from '../block-experiments/blocks/layout-grid/src';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a thought about the relative import path used here. Not suggesting a change, but wondering if there's a way to scope this (similar to how we resolve @wordpress dependencies) such that a future change in project structure wouldn't require any changes here. Something like '@something/blocks' 🤔

If there's too much "magic" in the module resolution configuration, perhaps we'd still achieve a similar effect by importing from an index at a higher level in ../block-experiments? It feels like "reaching deep into" the path here will be brittle to structure changes in the dependency. (This comment might belong there as well as here 😅 ) Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a great idea but I am not sure how to go about doing that. Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second idea would just involve creating an index.js (or possibly index.native.js) file in ../block-experiments/blocks/ and re-exporting the block registration there. This would shift the responsibility of maintaining the deeper path traversals to the dependency, which probably makes more sense, since that's where the structure changes would occur.

Regarding the module resolution magic, there is a plugin that might be worth investigating for this purpose, but I haven't tried it out. It essentially allows you to create an alias for the imports.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the module resolution magic, there is a plugin that might be worth investigating for this purpose, but I haven't tried it out. It essentially allows you to create an alias for the imports.

I've used that plugin in every React native project I've worked on, it's very useful. Also very easy to set up (usually).

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 3, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@enejb enejb force-pushed the add/experimental-grid-block-all-controls branch from bbc7b6b to 9f71767 Compare May 12, 2021 23:35
@enejb
Copy link
Contributor Author

enejb commented Feb 22, 2022

Closing this since it was already merged.

@enejb
Copy link
Contributor Author

enejb commented Feb 22, 2022

I am closing this since it is not needed any more.

@enejb enejb closed this Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants